Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecation notices #1769

Merged
merged 13 commits into from
Aug 18, 2015
Merged

Deprecation notices #1769

merged 13 commits into from
Aug 18, 2015

Conversation

fabpot
Copy link
Contributor

@fabpot fabpot commented Aug 16, 2015

Added a way to warn our users of deprecation features. This framework can be used for core features, but also for functions/tests/filters created by third-party. I haven't added something for tags as it can be done easily in the token parser class.

Fixes #1756 and #1767

@fabpot
Copy link
Contributor Author

fabpot commented Aug 16, 2015

Forgot to mention that this is WIP, docs are not finished yet, just placeholders :)

@stof
Copy link
Member

stof commented Aug 17, 2015

I haven't added something for tags as it can be done easily in the token parser class.

this would trigger it only during the template compilation, not at render time, so it may be hard.

@@ -448,6 +450,8 @@ public function resolveTemplate($names)
*/
public function clearTemplateCache()
{
@trigger_error(sprintf('%s is deprecated and will be removed in Twig 2.0.', __METHOD__), E_USER_DEPRECATED);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Symfony convention for deprecation messages is to say The method __METHOD__ is deprecated since X.Y and will be removed in Z.0. Use ... instead. (the second sentence being removed if there is no alternative). What do you think about being consistent in Twig ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to much details in Twig as all 1.x versions are basically interchangeable. Saying something was deprecated in 1.16 does not add anything useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tweaked some messages to make them look more Symfony-like.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the documentation also says as of 1.x, which does not make sense as we always introduced the replacement in a particular version

@stof
Copy link
Member

stof commented Aug 17, 2015

You should also trigger deprecation notices at the beginning of files for deprecated classes, as done in Symfony

@stof
Copy link
Member

stof commented Aug 17, 2015

TokenParserBroker related methods should also be marked as deprecated as the feature is deprecated.

new Twig_SimpleTest('same as', null, array('node_class' => 'Twig_Node_Expression_Test_Sameas')),
new Twig_SimpleTest('none', null, array('node_class' => 'Twig_Node_Expression_Test_Null')),
new Twig_SimpleTest('null', null, array('node_class' => 'Twig_Node_Expression_Test_Null')),
new Twig_SimpleTest('divisibleby', null, array('node_class' => 'Twig_Node_Expression_Test_Divisibleby')),
new Twig_SimpleTest('divisibleby', null, array('node_class' => 'Twig_Node_Expression_Test_Divisibleby', 'deprecated' => true, 'alternative' => 'divisibleby')),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternative is wrong, this should be: divisible by

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@fabpot
Copy link
Contributor Author

fabpot commented Aug 18, 2015

New version of the Twig_Deprecations class should be better now.

*/
public function collect(Traversable $iterator)
{
set_error_handler(function ($type, $msg) use (&$deprecations) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$deprecations is not defined yet (it is defined below).
Also closures don't work in PHP 5.2 (but is that really a problem?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, fixed.

PHP 5.2, hmmm... fixed

@fabpot fabpot force-pushed the deprecation-notices branch 2 times, most recently from a81901f to 23a1529 Compare August 18, 2015 10:16
{
private $iterator;

public function __construct(Traversable $iterator)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my comments on this class have been hidden by github because of the file renaming, but they are still valid

@fabpot fabpot force-pushed the deprecation-notices branch 2 times, most recently from ed77af1 to e8b9e48 Compare August 18, 2015 10:20

restore_error_handler();

return $this->deprecations;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should reset the array of deprecation in the object property here to avoid keeping a useless reference to the array

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm following here. You want me to add $this->deprecations = array();?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, so that the utility class does not keep state in memory longer than necessary. this way, the array of deprecation would not be referenced anymore by the collector once it is returned to the calling code

@fabpot fabpot force-pushed the deprecation-notices branch 2 times, most recently from 2f5723d to 481692c Compare August 18, 2015 11:30
@fabpot
Copy link
Contributor Author

fabpot commented Aug 18, 2015

Ready?

@stof
Copy link
Member

stof commented Aug 18, 2015

👍

@fabpot fabpot merged commit 56c7382 into twigphp:1.x Aug 18, 2015
fabpot added a commit that referenced this pull request Aug 18, 2015
This PR was merged into the 1.x branch.

Discussion
----------

Deprecation notices

Added a way to warn our users of deprecation features. This framework can be used for core features, but also for functions/tests/filters created by third-party. I haven't added something for tags as it can be done easily in the token parser class.

Fixes #1756 and #1767

Commits
-------

56c7382 added Twig_Util_DeprecationCollector to collect deprecation notices for a set of templates
2da6bae tweaked some deprecation messages
cc98028 added deprecation notices for deprecated classes
c6ab7de optimized legacy tests management
eebfdc7 added a way to manage legacy tests in fixtures
8c4291a removed usage of deprecated features, marked some tests as being legacy
b5db9dc added the Symfony PHPUnit bridge to better manage deprecation notices
23c3922 added deprecated notices for deprecated features
bd87ba8 added some documentation about deprecation notices
d16b7bf added a way to trigger deprecation notices for filters and functions
21f536b added deprecation for the raw tag
efcfe8e added deprecation notices for deprecated tests
65e70a2 fixed CS
template names as keys and template contents as values (as done by
``Twig_Util_TemplateDirIterator``).

However, this code won't find all deprecations (like using deprecated some Twig
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like using some deprecated Twig instead of like using deprecated some Twig

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants